Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Code: Firewall filter #416

Merged
merged 3 commits into from
Oct 28, 2021

Conversation

markmandel
Copy link
Member

What type of PR is this?

Uncomment only one /kind <> line, press enter to put that in a new line, and remove leading whitespace from that line:

/kind breaking
/kind bug
/kind cleanup
/kind documentation

/kind feature

/kind hotfix

What this PR does / Why we need it:

Code implementation of a Firewall filter that will allow/deny packets based on their from address on both read and write.

Which issue(s) this PR fixes:

Work on #158
Work on #343

Special notes for your reviewer:

Documentation to come next to finish off the above two tickets.

Also have some questions on a couple of places that I think would be good to get confirmation on.

@markmandel markmandel added kind/feature New feature or request area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc labels Oct 7, 2021
@google-cla google-cla bot added the cla: yes label Oct 7, 2021
@github-actions github-actions bot added the size/l label Oct 7, 2021
@markmandel
Copy link
Member Author

/cc @davidkornel who might be interested.

}
}

debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if no rules match, I default to not allowing a packet through. Is this acceptable?

I see a few options:

  1. This is fine
  2. If someone wants to allow all through by default, they can use an "everything" cidr range to do so.
  3. We add some sort of "default" allow/deny option to the config for on_read and on_write.

I'm happy to leave things as is for the time being, and see if anyone has a preference, but figured I would highlight this and ask the question.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behaviour of other firewall projects here, what happens in Envoy, firewalld, iptables, etc? I think that will probably give us an answer.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking denying by default could be fine depending on what guarantees we're providing which we should mention in the docs - if we guarantee that we'll validate according to the rules list order then yeah it should be fine and they can have a catch all range at the end of the list.
If we don't guarantee the order rules are checked against (which would enable us to optimizations to e.g support when we have lots of rules and want to avoid scanning) then having an explicit 'default allow/deny' option might be preferrable.

Coming to think of it (probably not for this PR), we should also document the behavior when a user specifies conflicting port ranges in different rules e.g allow 10-20 in one rule and deny 20-30 in another - e.g we could treat the config as invalid

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm struggling to find an example with Envoy where it will do firewall filtering (I'm not sure it can? but it's entirely possible I'm lost in it's docs), but i did find it in Istio:
https://istio.io/latest/docs/reference/config/security/authorization-policy/ which has an interesting formula of decision making, but if nothing is applied, it does deny in the end (I personally think that checking and matching in the order configured is easier to understand, but maybe that's just me).

Authorization policy supports CUSTOM, DENY and ALLOW actions for access control. When CUSTOM, DENY and ALLOW actions are used for a workload at the same time, the CUSTOM action is evaluated first, then the DENY action, and finally the ALLOW action. The evaluation is determined by the following rules:

If there are any CUSTOM policies that match the request, evaluate and deny the request if the evaluation result is deny.
If there are any DENY policies that match the request, deny the request.
If there are no ALLOW policies for the workload, allow the request.
If any of the ALLOW policies match the request, allow the request.
Deny the request.

For iptables, the default is ACCEPT, but you can configure it to be DENY by default, there's a setting. I thought about having there be a top level default option, but wasn't sure if it was worth it? (or could be added later).

https://superuser.com/questions/437013/what-does-an-empty-iptables-mean
https://servercomputing.blogspot.com/2012/03/change-iptables-default-policy-to-drop.html

But definitely taking note that this needs to be spelling out very clearly in the docs, which will come once this PR is merged).


Ok(Metrics {
packets_denied_on_read: deny_metric
.get_metric_with_label_values(vec!["on_read"].as_slice())?,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than "on_read" and "on_write" this could be "read" and "write" ?

I figured it would be good to set a precedent here, as we might do this in a few different places.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me. (I'm thinking we should just call the actual filter methods on_read and on_write as well to match)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it would be good to set a precedent here, as we might do this in a few different places.

I think switching it is fine, but I kinda wish we had a better, more typed API for getting and creating the metric labels so that it's easier to have it always be the right thing.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but I kinda wish we had a better, more typed API for getting and creating the metric labels so that it's easier to have it always be the right thing.

🤔 Yeah me too. I moved this into some consts since they get reused, and it'll be easier to put somewhere common if we need to later.

Copy link
Collaborator

@iffyio iffyio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments, overall I think this looks good!
We should include documentation for the added feature though (preferably in this PR since it'll be easier to review the docs that way)

Comment on lines 80 to 139
#[derive(Clone, Debug, PartialEq)]
pub struct PortRange {
pub min: u16,
pub max: u16,
}

impl PortRange {
/// Does this range contain a specific port value?
///
/// # Arguments
///
/// * `port`:
///
/// returns: bool
pub fn contains(&self, port: u16) -> bool {
port >= self.min && port <= self.max
}
}

impl Serialize for PortRange {
/// Serialise the [PortRange] into a single digit if min and max are the same
/// otherwise, serialise it to "min-max".
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
if self.max == self.min {
return serializer.serialize_str(self.min.to_string().as_str());
}

let range = format!("{}-{}", self.min, self.max);
serializer.serialize_str(range.as_str())
}
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can serialize directly into rust's range type instead of introducing ours? https://doc.rust-lang.org/std/ops/struct.Range.html

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh and @XAMPPRocky 's idea of using rust's range syntax wherever we need these types could be nice to use as well wdyt? #404 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@XAMPPRocky made the same comment about using a Range - that's an excellent point, I will make that change.

So the format for the port range I directly copied from how we do port ranges in the GCP Firewalls:
https://cloud.google.com/vpc/docs/firewalls#:~:text=protocol%20and%20port%20range

AWS does similar:
https://docs.aws.amazon.com/AWSEC2/latest/UserGuide/security-group-rules.html#:~:text=for%20example%2C%207000-8000

I figured it would be good to stick with something that people know from configuring firewall rules in existing systems.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I figured it would be good to stick with something that people know from configuring firewall rules in existing systems.

That sounds very reasonable to me 👍

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using the newtype pattern to wrap the Range, for two reasons:

If I used deserialize_with then I would have had to deserialize the entire Vector, which seems like too much work.

The other side effect was it forced me to write a PortRange::new(min: u16, max: u16) -> Result<Self, PortRangeError> which moved all the validation logic for the range into the constructor, rather than handling it in the parsing rules - which removes the repetition, and makes a better API surface if anyone else wants to use it directly.

Lemme know what you think 👍🏻

Comment on lines +171 to +188
ConvertProtoConfigError::new(
format!("min too large: {}", err),
Some("port.min".into()),
)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we likely need to do the validation after we have deserialized into the PortRange struct, so that we can avoid duplicating the validation logic?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be better now since we have PortRange::new(...) -> Result, and the validation is in there.


#[derive(Clone, Deserialize, Debug, PartialEq, Serialize)]
pub enum Action {
/// Matching details will allow packets through.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Matching details will allow packets through.
/// Matching rules will allow packets through.

/// Matching details will allow packets through.
#[serde(rename = "ALLOW")]
Allow,
/// Matching details will block packets.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// Matching details will block packets.
/// Matching rules will block packets.


impl Metrics {
pub(super) fn new(registry: &Registry) -> MetricsResult<Self> {
let event_labels = vec!["events"];
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let event_labels = vec!["events"];
let event_labels = vec!["event"];


Ok(Metrics {
packets_denied_on_read: deny_metric
.get_metric_with_label_values(vec!["on_read"].as_slice())?,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That sounds reasonable to me. (I'm thinking we should just call the actual filter methods on_read and on_write as well to match)

Comment on lines 85 to 102
for rule in &self.on_read {
if rule.contains(ctx.from) {
return match rule.action {
Action::Allow => {
debug!(self.log, "Allow"; "event" => "read", "from" => ctx.from.to_string());
self.metrics.packets_allowed_on_read.inc();
Some(ctx.into())
}
Action::Deny => {
debug!(self.log, "Deny"; "event" => "read", "from" => ctx.from );
self.metrics.packets_denied_on_read.inc();
None
}
};
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The loop and rules checking seem identical to both read and write, we can move it to its own function and pass in arguments that differ like the vec, eventtype, metrics?

}
}

debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking denying by default could be fine depending on what guarantees we're providing which we should mention in the docs - if we guarantee that we'll validate according to the rules list order then yeah it should be fine and they can have a catch all range at the end of the list.
If we don't guarantee the order rules are checked against (which would enable us to optimizations to e.g support when we have lots of rules and want to avoid scanning) then having an explicit 'default allow/deny' option might be preferrable.

Coming to think of it (probably not for this PR), we should also document the behavior when a user specifies conflicting port ranges in different rules e.g allow 10-20 in one rule and deny 20-30 in another - e.g we could treat the config as invalid

Copy link
Collaborator

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Mostly style and tidy comments.

}
}

debug!(self.log, "default: Deny"; "event" => "read", "from" => ctx.from.to_string());
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the behaviour of other firewall projects here, what happens in Envoy, firewalld, iptables, etc? I think that will probably give us an answer.

Comment on lines 141 to 154
let firewall = Firewall {
log: logger(),
metrics: Metrics::new(&Registry::default()).unwrap(),
on_read: vec![Rule {
action: Action::Allow,
source: "192.168.75.0/24".parse().unwrap(),
ports: vec![PortRange { min: 10, max: 100 }],
}],
on_write: vec![],
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you call Firewall::new here instead? Then if you later update Firewall with a new field from Config you don't have to also update these tests.


let ctx = ReadContext::new(
UpstreamEndpoints::from(
Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using parse here is a bit of anti-pattern as we're doing string parsing when we construct valid sockets with ([u8; 4], u16)/(Ipv4Addr, u16).

Suggested change
Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
Endpoints::new(vec![Endpoint::new((Ipv4Addr::LOCALHOST, 8080).into())]).unwrap(),

UpstreamEndpoints::from(
Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
),
"192.168.75.20:80".parse().unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"192.168.75.20:80".parse().unwrap(),
([192, 168, 75, 20], 80).into(),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not know you could do that! Nice!

Comment on lines 165 to 169
Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
),
"192.168.75.20:2000".parse().unwrap(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add the addresses from above as consts and reference them here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved them into variables. Was a bit easier for ownership.

Comment on lines 28 to 31
use super::quilkin::extensions::filters::firewall::v1alpha1::firewall::{
Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule,
};
use super::quilkin::extensions::filters::firewall::v1alpha1::Firewall as ProtoConfig;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use super::quilkin::extensions::filters::firewall::v1alpha1::firewall::{
Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule,
};
use super::quilkin::extensions::filters::firewall::v1alpha1::Firewall as ProtoConfig;
use super::quilkin::extensions::filters::firewall::v1alpha1::{
Firewall as ProtoConfig,
firewall::{Action as ProtoAction, PortRange as ProtoPortRange, Rule as ProtoRule},
};

Comment on lines 17 to 20
use std::convert::TryFrom;
use std::fmt;
use std::fmt::Formatter;
use std::net::SocketAddr;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd reduce these to a single import. People still write code like this because nested imports were a later addition.

Suggested change
use std::convert::TryFrom;
use std::fmt;
use std::fmt::Formatter;
use std::net::SocketAddr;
use std::{convert::TryFrom, fmt::{self, Formatter}, net::SocketAddr};

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh! I like it! Today I also learned the IDEA Rust can do this for you 👍

Comment on lines 20 to 24
use crate::filters::firewall::config::{Action, Config, Rule};
use crate::filters::firewall::metrics::Metrics;
use crate::filters::{
CreateFilterArgs, DynFilterFactory, Error, Filter, FilterFactory, FilterInstance, ReadContext,
ReadResponse, WriteContext, WriteResponse,
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
use crate::filters::firewall::config::{Action, Config, Rule};
use crate::filters::firewall::metrics::Metrics;
use crate::filters::{
CreateFilterArgs, DynFilterFactory, Error, Filter, FilterFactory, FilterInstance, ReadContext,
ReadResponse, WriteContext, WriteResponse,
};
use crate::filters::prelude::*;
use self::{config::{Action, Config, Rule}, metrics::Metrics};

src/filters/firewall.rs Show resolved Hide resolved
let min = split.0.parse::<u16>().map_err(de::Error::custom)?;
let max = split.1.parse::<u16>().map_err(de::Error::custom)?;

if min > max {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pointing out here that this does allow effective ranges of length zero where min == max, not sure if that's supposed to be intentional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's intentional - it's the equivalent of having a single port value 👍🏻

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes fix this, so it can't be this way anymore.

@markmandel
Copy link
Member Author

Thanks for the detailed feedback, I'll take some time to go through it all and make adjustments 👍🏻

@markmandel markmandel force-pushed the feature/firewall-filter branch from 5c9f500 to 9bfd5e2 Compare October 19, 2021 03:39
Copy link
Member Author

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still working through all the comments, but I think this is looking better! Thanks for all the input.

Comment on lines 80 to 139
#[derive(Clone, Debug, PartialEq)]
pub struct PortRange {
pub min: u16,
pub max: u16,
}

impl PortRange {
/// Does this range contain a specific port value?
///
/// # Arguments
///
/// * `port`:
///
/// returns: bool
pub fn contains(&self, port: u16) -> bool {
port >= self.min && port <= self.max
}
}

impl Serialize for PortRange {
/// Serialise the [PortRange] into a single digit if min and max are the same
/// otherwise, serialise it to "min-max".
fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
where
S: Serializer,
{
if self.max == self.min {
return serializer.serialize_str(self.min.to_string().as_str());
}

let range = format!("{}-{}", self.min, self.max);
serializer.serialize_str(range.as_str())
}
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I ended up using the newtype pattern to wrap the Range, for two reasons:

If I used deserialize_with then I would have had to deserialize the entire Vector, which seems like too much work.

The other side effect was it forced me to write a PortRange::new(min: u16, max: u16) -> Result<Self, PortRangeError> which moved all the validation logic for the range into the constructor, rather than handling it in the parsing rules - which removes the repetition, and makes a better API surface if anyone else wants to use it directly.

Lemme know what you think 👍🏻

Comment on lines +171 to +188
ConvertProtoConfigError::new(
format!("min too large: {}", err),
Some("port.min".into()),
)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be better now since we have PortRange::new(...) -> Result, and the validation is in there.

Comment on lines 17 to 20
use std::convert::TryFrom;
use std::fmt;
use std::fmt::Formatter;
use std::net::SocketAddr;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oooh! I like it! Today I also learned the IDEA Rust can do this for you 👍

let min = split.0.parse::<u16>().map_err(de::Error::custom)?;
let max = split.1.parse::<u16>().map_err(de::Error::custom)?;

if min > max {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new changes fix this, so it can't be this way anymore.

@markmandel markmandel force-pushed the feature/firewall-filter branch from 97a5752 to cbc680e Compare October 22, 2021 04:29
Copy link
Member Author

@markmandel markmandel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Finished my updates - should be good for a second review.

Comment on lines 165 to 169
Endpoints::new(vec![Endpoint::new("127.0.0.1:8080".parse().unwrap())]).unwrap(),
),
"192.168.75.20:2000".parse().unwrap(),
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I moved them into variables. Was a bit easier for ownership.

Comment on lines 77 to 78
/// InitializeError is returned with an error message if the
/// [`ClusterManager`] fails to initialize properly.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
/// InitializeError is returned with an error message if the
/// [`ClusterManager`] fails to initialize properly.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Whoops! Nice catch. Copy paste error! Removed!

@markmandel markmandel force-pushed the feature/firewall-filter branch from cbc680e to c8ab12d Compare October 25, 2021 22:05
@markmandel
Copy link
Member Author

🤞🏻 this should be good to go now.

Copy link
Collaborator

@XAMPPRocky XAMPPRocky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Left some documentation comments but feel free to address in a future PR.

tests/firewall.rs Outdated Show resolved Hide resolved
src/filters/firewall.rs Show resolved Hide resolved
src/filters/firewall/config.rs Outdated Show resolved Hide resolved
src/filters/firewall/config.rs Outdated Show resolved Hide resolved
src/filters/firewall/config.rs Show resolved Hide resolved
@markmandel
Copy link
Member Author

@XAMPPRocky that's all excellent feedback (and I totally have a habit of writing Go style comments) -- lemme incorporate that all in before it gets merged 👍🏻

Code implementation of a Firewall filter that will allow/deny packets
based on their from address on both read and write.

Documentation to come next to finish off the below two tickets.

Work on googleforgames#158
Work on googleforgames#343
@markmandel markmandel force-pushed the feature/firewall-filter branch from c8ab12d to 4301ad5 Compare October 26, 2021 21:49
@markmandel
Copy link
Member Author

Just wanted to check before merging if there was anything else? Otherwise I will merge and start writing the docs. 👍🏻

@quilkin-bot
Copy link
Collaborator

Build Succeeded 🥳

Build Id: 75592aae-05e5-4de7-8f40-60c4e2dccbe8

To build this version:

git fetch [email protected]:googleforgames/quilkin.git pull/416/head:pr_416 && git checkout pr_416
cargo build

@XAMPPRocky XAMPPRocky merged commit 5d713e5 into googleforgames:main Oct 28, 2021
@markmandel markmandel deleted the feature/firewall-filter branch November 2, 2021 22:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/user-experience Pertaining to developers trying to use Quilkin, e.g. cli interface, configuration, etc cla: yes kind/feature New feature or request size/l
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants